Skip to content

gzip filter: allow setting zlib compressor's chunk size#10508

Merged
mattklein123 merged 22 commits intoenvoyproxy:masterfrom
rgs1:gzip-filter-allow-setting-zlib-next-output-buffer
Apr 25, 2020
Merged

gzip filter: allow setting zlib compressor's chunk size#10508
mattklein123 merged 22 commits intoenvoyproxy:masterfrom
rgs1:gzip-filter-allow-setting-zlib-next-output-buffer

Conversation

@rgs1
Copy link
Member

@rgs1 rgs1 commented Mar 25, 2020

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

gzip filter: allow setting zlib compressor's chunk size
6adfd9d 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1 rgs1 requested a review from dio as a code owner March 25, 2020 00:24
@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Mar 25, 2020

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10508 was opened by rgs1.

see: more, trace.

@rgs1
Copy link
Member Author

rgs1 commented Mar 25, 2020

fwiw, nginx sets this value to 8kb. see:

http://nginx.org/en/docs/http/ngx_http_gzip_module.html#gzip_buffers

@rgs1 rgs1 mentioned this pull request Mar 25, 2020
Open
dio
dio reviewed Mar 25, 2020
View reviewed changes
Update docs
424fe41 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1 rgs1 mentioned this pull request Mar 25, 2020
Merged
@rgs1
Copy link
Member Author

rgs1 commented Mar 25, 2020

/retest

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Mar 25, 2020

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10508 (comment) was created by @rgs1.

see: more, trace.

@dio
Copy link
Member

dio commented Mar 25, 2020 •
edited
Loading

Looks good. For the API review, @lizan or maybe @htuch, could you help with this? Thanks!

rojkov
rojkov reviewed Mar 25, 2020
View reviewed changes
htuch
htuch reviewed Mar 26, 2020
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API generally looks good, just a couple of clarifying questions.

@stale
Copy link

stale bot commented Apr 2, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added stale stalebot believes this issue/PR has not been touched recently and removed stale stalebot believes this issue/PR has not been touched recently labels Apr 2, 2020
Raul Gutierrez Segales added 3 commits April 2, 2020 15:24
Merge remote-tracking branch 'upstream/master' into gzip-filter-allow…
3604115 
…-setting-zlib-next-output-buffer
google.protobuf.UInt64Value --> UInt32Value
3aa1ae3 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
proto: fix validation rules
1d3aec3 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@htuch
Copy link
Member

htuch commented Apr 3, 2020

API LGTM, curious how this relates to #10553? CC @rojkov

@rgs1
Copy link
Member Author

rgs1 commented Apr 3, 2020

API LGTM, curious how this relates to #10553? CC @rojkov

We probably want to expose this there as well, but given it'll take a while for the gzip filter to go away we could start here.

@rojkov
Copy link
Member

rojkov commented Apr 3, 2020

This PR looks good. I'm going to extend #10553 when this one is merged.

@htuch
Copy link
Member

htuch commented Apr 3, 2020

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Apr 3, 2020
lizan
lizan reviewed Apr 3, 2020
View reviewed changes
@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 11, 2020
Raul Gutierrez Segales added 2 commits April 13, 2020 10:33
Merge remote-tracking branch 'upstream/master' into gzip-filter-allow…
9b5c43a 
…-setting-zlib-next-output-buffer
Delete old version history file
a977e5b 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 13, 2020
@dio
Copy link
Member

dio commented Apr 16, 2020

Seems like we need to merge master.

Raul Gutierrez Segales added 3 commits April 16, 2020 13:04
Merge remote-tracking branch 'upstream/master' into gzip-filter-allow…
2b86067 
…-setting-zlib-next-output-buffer
Fix history
88bb09c 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Sync protos
e4b7161 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1
Copy link
Member Author

rgs1 commented Apr 16, 2020

Unrelated failure:

+ GCOV_PREFIX=/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/_coverage/test/coverage/coverage_tests/shard_27_of_50/test
+ export LLVM_PROFILE_FILE=/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/_coverage/test/coverage/coverage_tests/shard_27_of_50/test/%h-%p-%m.profraw
+ LLVM_PROFILE_FILE=/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/_coverage/test/coverage/coverage_tests/shard_27_of_50/test/%h-%p-%m.profraw
+ [[ ! -z '' ]]
+ cd /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/bazel-out/k8-fastbuild/bin/test/coverage/coverage_tests.runfiles/envoy
+ /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/bazel-out/k8-fastbuild/bin/test/coverage/coverage_tests.runfiles/envoy/test/coverage/coverage_tests --gmock_default_mock_behavior=2 '--log-path /dev/null' '-l trace'
external/bazel_tools/tools/test/collect_coverage.sh: line 131: 30183 Segmentation fault      (core dumped) "$@"
+ TEST_STATUS=139
+ touch /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/coverage/coverage_tests/shard_27_of_50/coverage.dat
+ [[ 139 -ne 0 ]]
+ echo --

@rgs1
Copy link
Member Author

rgs1 commented Apr 16, 2020

/retest

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Apr 16, 2020

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10508 (comment) was created by @rgs1.

see: more, trace.

Raul Gutierrez Segales added 2 commits April 20, 2020 11:34
Merge remote-tracking branch 'upstream/master' into gzip-filter-allow…
59a261b 
…-setting-zlib-next-output-buffer
Update version history
4a4d6be 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@htuch
Copy link
Member

htuch commented Apr 24, 2020

/lgtm api

@mattklein123 mattklein123 self-assigned this Apr 24, 2020
mattklein123
mattklein123 requested changes Apr 24, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with small comments. Thank you!

/wait

Raul Gutierrez Segales added 3 commits April 24, 2020 18:15
Merge remote-tracking branch 'upstream/master' into gzip-filter-allow…
75f640e 
…-setting-zlib-next-output-buffer
Use PROTOBUF_GET_WRAPPED_OR_DEFAULT
391f978 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Revert v2 api changes, we are v3 only now
644b14c 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1 rgs1 force-pushed the gzip-filter-allow-setting-zlib-next-output-buffer branch from 9ab99a9 to 644b14c Compare April 24, 2020 22:32
@rgs1
Copy link
Member Author

rgs1 commented Apr 24, 2020 •
edited
Loading

@mattklein123 sorry about the force push :-(

I made the mistake of fixing a merge conflict from the UI and forgot to pull back.

Raul Gutierrez Segales added 2 commits April 24, 2020 19:03
Fix
f10da4d 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Fix of the fix
703e368 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
mattklein123
mattklein123 approved these changes Apr 24, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit 0b0213f into envoyproxy:master Apr 25, 2020
spenceral added a commit to spenceral/envoy that referenced this pull request Apr 27, 2020
@spenceral
Merge branch 'master' into health-check-transport-match
f485e79 
Signed-off-by: Spencer Lewis <slewis@squareup.com>

* master:
  fault injection: add support for setting gRPC status (envoyproxy#10841)
  tests: tag tests that fail on Windows with fails_on_windows (envoyproxy#10940)
  Fix typo on Postgres Proxy documentation. (envoyproxy#10930)
  fuzz: improve header/data stop/continue modeling in HCM fuzzer. (envoyproxy#10931)
  gzip filter: allow setting zlib compressor's chunk size (envoyproxy#10508)
  http: replace vector/reserve with InlinedVector in codec helper (envoyproxy#10941)
  stats: add utilities to create stats from a vector of tokens, mixing dynamic and symbolic elements. (envoyproxy#10735)
  hcm: avoid invoking 100-continue handling on decode filter. (envoyproxy#10929)
  prometheus stats: Correctly group lines of the same metric name. (envoyproxy#10833)
  status: Fix ASAN error in Status payload handling (envoyproxy#10906)
  path: Fix merge slash for paths ending with slash and present query args (envoyproxy#10922)
  compressor filter: add benchmark (envoyproxy#10464)
  xray: expected_span_name is not captured by the lambda with MSVC (envoyproxy#10934)
  ci: update before purge in cleanup (envoyproxy#10938)
  tracer: Improve test coverage for x-ray (envoyproxy#10890)
  Revert "init: order dynamic resource initialization to make RTDS always be first (envoyproxy#10362)" (envoyproxy#10919)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

+4 more reviewers

@rojkov rojkov rojkov left review comments

@dio dio dio left review comments

@lizan lizan lizan left review comments

@htuch htuch htuch left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@dio dio

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@rgs1 @dio @htuch @rojkov @lizan @mattklein123